fix(decode): stop infinite loop on non-object repeated message elements - #6

Problem

A tiny untrusted payload can make decoding loop until OutOfMemoryError:

BuffJson.decoder().decode("{\"repeatedNested\":[1]}", TestNesting.class); // never returns, then OOM

When a message reader met a non-object token, it returned an empty message without consuming the token. It ignored nextIfObjectStart()'s result, and readFieldName() returns null on a non-name token, which ended the field loop. Inside a repeated field, the enclosing while (!reader.nextIfArrayEnd()) loop then saw the same token forever, appending messages until the heap was exhausted.

Inputs that looped before this change (-Xmx96m, 8 s timeout):

Singular and map values were rejected only by accident: the leftover token made the top-level "input not end" check fail. The official conformance cases for null repeated elements appeared to pass only because ConformanceTestee catches Throwable, which reported the OutOfMemoryError as a parse failure.

Fix

1. Every reader consumes the container it expects, or throws a JSONException that names the proto type:

• { for messages, maps, Struct, Any and Empty;
• [ for repeated fields and ListValue.

The checks live in two helpers, FieldReader.requireObjectStart and requireArrayStart, and the generated decoders call them too. So all three paths produce the same error, for example Expected a JSON object for message io.suboptimal.buffjson.proto.NestedMessage, offset 32, …. Changed code:

• DecoderGenerator — message, repeated, map and inline Empty reads
• ProtobufMessageReader, TypedMessageReaderSchema, FieldReader.readRepeated/readMap
• WellKnownTypes.readStruct/readListValue/readAny

Every element read now consumes at least its opening token, so each iteration of an array loop makes progress. Termination no longer depends on what the element reader does. The checks test a boolean the reader already returned, so there is no cost on the success path. The BuffJsonGeneratedDecoder.readMessage Javadoc now requires this consume-or-throw behavior.

2. Null elements in repeated fields, found by the same test matrix:

• Codegen now rejects a null element with a JSONException (FieldReader.requireNonNullElement), as JsonFormat does. Before, it threw NullPointerException (Timestamp, Duration, FieldMask, String/BytesValue) or added a phantom default element (int64, bool, enum, wrappers).
• google.protobuf.Value / NullValue elements keep null as a value on every path (a wrapped NullValue, or NULL_VALUE). The typed and reflection paths used to drop them, so {"repeatedValue":[null,1]} lost an element, even though that is exactly what the encoder writes.

3. Null map values of Value / NullValue type are kept on every path (FieldReader.nullValueFor), as JsonFormat and the encoder expect. Before, codegen dropped the entry, and the runtime paths stored an empty Value with no kind set, so a round trip lost data. A single shared WellKnownTypes.NULL_JSON_VALUE now serves every reader.

4. Empty input. Empty or whitespace-only input decodes to null from every overload (byte[], slice, InputStream), like an empty String. This is now documented on BuffJsonDecoder. Without it, the new object-start check would have turned these inputs into exceptions.

Behavior changes (invalid, empty or null input only)

• Mismatched containers: non-object and non-array containers now fail with a clear JSONException instead of looping or failing on the trailing-input check. Valid proto3 JSON always uses these shapes, and JsonFormat rejects everything else.
• Codegen Empty: a bare number or array for google.protobuf.Empty is now rejected, matching the runtime paths.
• Codegen null elements are now rejected (see 2).
• Null Value/NullValue elements and map values are kept as values on every path (see 2 and 3).
• Empty input: empty byte[]/InputStream or whitespace-only input now returns null. On main it returned an empty message; an empty String already returned null.

Tests

• New BuffJsonMalformedContainerTest: each case runs on all three decode paths inside assertTimeoutPreemptively. It covers:

  • non-object repeated message, Struct, ListValue, Any and Empty elements;
  • non-array repeated values;
  • non-object map values, singular message values and top-level input;
  • malformed objects inside containers;
  • truncated input and null repeated message elements (termination only);
  • null scalar and WKT elements;
  • repeated Value, repeated NullValue, map<_, Value> and map<_, NullValue> nulls, checked against JsonFormat plus an encoder round trip;
  • empty input from every overload.

  Against the unfixed code, the forked test JVM died with Java heap space.

• New test messages: TestRepeatedNullValue and TestNullMapValues were added to the test protos so the generated null branches are compiled and exercised.

• mvn -B clean verify: all modules pass (730 tests in buff-json-tests), including the benchmark and conformance-testee builds.

• Not run locally: the official conformance_test_runner. CI runs it for all three BUFFJSON_PATHs.

Consumers must rebuild with mvn clean install to regenerate decoders. The protobuf Maven plugin only regenerates when .proto inputs change.

Not changed (possible follow-ups)

• Decoders generated by the old plugin still loop, because generated decoders call each other directly and no runtime guard can reach them. They need regeneration, or a plugin/runtime version check.
• Null elements in non-Value repeated fields are still skipped by the typed and reflection paths, while codegen rejects them.
• Null map values of other types: codegen drops the entry, while the runtime paths insert the default value, and neither matches JsonFormat. runtimeNullMapValuesAndUnknownEnumNumbersMatchReflection pins the runtime behavior, so this needs a decision on which is intended.
• A member without a name still ends an object early, as before. So an unclosed object such as [{"a":1, {"a":2}] is still accepted as two elements. The existing truncated-object leniency (BuffJsonErrorTest.truncatedObjectParsesLeniently) is unchanged.
• Pre-existing issues seen while probing, unrelated to this diff:
  • readUnsignedLong throws NullPointerException for an object element ({"repeatedUint64":[{}]}).
  • fastjson2 silently coerces some wrong-typed elements: [{}] or [1.5] for an enum, [true] for a double, [1] for a StringValue.
  • fastjson2 treats a leading U+001A as end of input.
• assertTimeoutPreemptively can't stop a runaway decode thread. If the fix regresses, the test fails at the timeout, but the fork may still run out of memory afterwards.

🤖 Generated with Claude Code

https://claude.ai/code/session_013AbdzHqfNXSdynywckLnWY